Skip to content

Develop - #74

Merged
jaebeom79 merged 4 commits into
mainfrom
develop
May 19, 2026
Merged

Develop#74
jaebeom79 merged 4 commits into
mainfrom
develop

Conversation

@jaebeom79

Copy link
Copy Markdown
Contributor

📢 기능 설명

필요시 실행결과 스크린샷 첨부

연결된 issue

연결된 issue를 자동을 닫기 위해 아래 {이슈넘버}를 입력해주세요.

close #{이슈넘버}

✅ 체크리스트

  • PR 제목 규칙 잘 지켰는가?
  • 추가/수정사항을 설명하였는가?
  • 이슈넘버를 적었는가?

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a comprehensive monitoring stack including Prometheus, Loki, Promtail, Jaeger, and Grafana, alongside OpenTelemetry integration for automated tracing. It also optimizes the StoreRemain slot generation logic by implementing pre-calculated slot plans and using interface projections to improve database query efficiency. Security improvements include restricting external access to Actuator endpoints via Nginx. Review feedback suggests evaluating alternative Garbage Collectors (Serial or Parallel) for the small heap size, mitigating security risks by avoiding the root user for the Jaeger container, and implementing null checks before parsing store operating hours to prevent potential exceptions.

Comment thread Dockerfile
ENV JAVA_TOOL_OPTIONS="-XX:MaxRAMPercentage=70.0 -XX:InitialRAMPercentage=50.0"
# t3.small 환경 고려한 JVM 메모리 튜닝 + OTel Agent 자동 활성화
# OTEL_EXPORTER_OTLP_ENDPOINT가 설정 안 되면 트레이스 전송만 실패할 뿐 앱은 정상 동작
ENV JAVA_TOOL_OPTIONS="-javaagent:/opentelemetry-javaagent.jar -XX:MaxRAMPercentage=70.0 -XX:InitialRAMPercentage=50.0 -XX:+UseG1GC"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

While -XX:+UseG1GC is the default in Java 21, explicitly setting it for a container limited to 600MB (as seen in docker-compose.prod.yml) might not be optimal. G1GC is generally more effective for larger heaps. For a heap size around 400MB (70% of 600MB), you might consider testing SerialGC or ParallelGC to see if they provide better performance with lower overhead in this resource-constrained environment.

Comment thread docker-compose.prod.yml
jaeger:
image: jaegertracing/all-in-one:1.62.0
container_name: catchtable-jaeger
user: "0" # named volume mkdir 퍼미션 회피 (내부 도구라 보안 영향 없음)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Running the Jaeger container as root (user: "0") is a security risk, even for internal tools. It is better to ensure the host directory has the correct permissions for the container's default user or to specify a non-root UID that matches the host's directory owner.

Comment on lines +154 to +155
LocalTime openTime = LocalTime.parse(store.getOpenTime(), TIME_FORMATTER);
LocalTime closeTime = LocalTime.parse(store.getCloseTime(), TIME_FORMATTER);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The LocalTime.parse method will throw a NullPointerException if store.getOpenTime() or store.getCloseTime() returns null. Although there is a catch block, it is safer and more idiomatic to perform a null check before parsing to avoid unnecessary exception overhead.

                String openTimeStr = store.getOpenTime();
                String closeTimeStr = store.getCloseTime();
                if (openTimeStr == null || closeTimeStr == null) {
                    log.warn("[영업시간 누락] storeId={}", store.getId());
                    continue;
                }
                LocalTime openTime = LocalTime.parse(openTimeStr, TIME_FORMATTER);
                LocalTime closeTime = LocalTime.parse(closeTimeStr, TIME_FORMATTER);

@jaebeom79
jaebeom79 merged commit ca04aac into main May 19, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants